-
-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUGFIX: Fix and test tethered node structure adjustment #4969
base: 9.0
Are you sure you want to change the base?
BUGFIX: Fix and test tethered node structure adjustment #4969
Conversation
Currently fails with
this is due to the blocking in |
break; | ||
} | ||
/** @var Node $childNodeSource Node aggregates are never empty */ | ||
$occupiedDimensionSpacePoints = $childNodeAggregate->occupiedDimensionSpacePoints->getPoints(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk if we should guard here with this check ... the added tests will pass either way ;)
if (count($occupiedDimensionSpacePoints) !== 1) {
throw new \RuntimeException('TODO: The ChildNodeAggregate occupies multiple origins.', 1711897662);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see an issue with the child node aggregate occupying multiple DSPs, that's totally legit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be synced with 9.0, looks otherwise fine so far
break; | ||
} | ||
/** @var Node $childNodeSource Node aggregates are never empty */ | ||
$occupiedDimensionSpacePoints = $childNodeAggregate->occupiedDimensionSpacePoints->getPoints(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see an issue with the child node aggregate occupying multiple DSPs, that's totally legit
✅ |
@@ -33,4 +35,31 @@ public static function empty(): self | |||
ExpectedVersion::ANY() | |||
); | |||
} | |||
|
|||
public function withCausationOfFirstEventAndAdditionalMetaData(EventMetadata $metadata): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that might be something were i guess only @bwaidelich can say if this is a good idea.
I use this utility to tie all events together and add a description to the first event as to the why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea in general (thus #3887) but we have to get this right:
The first event is not really the cause for the other events but the command. Our commands don't have an id yet. We could actually change that since we persist the commands.
But instead we should IMO focus on the correlation id (so that all events of one transaction are inter-connected).
The causation id does not have to be a UUID – we could e.g. add some speaking prefix, for example the command type (see #3887 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay thanks so i understand all should share one common correlationId
(CorrelationId::fromString(UuidFactory::create()
).
But additionally i want to add somewhere to the (first?) event (as part of metadata?) which structure adjustment caused the creation of this event.
So that the events read like:
Name | correlation id | metadata |
---|---|---|
NodeWasCreated | 123 | {structureAdjustment: 'Content Stream: %s; Dimension Space Point: %s, Node Aggregate: %s --- The tethered child node "bar" is missing.'} |
SomethingLol | 123 |
at first i tried to insert the rendered structure adjustment message in there but that wont work because its to long and special chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that makes sense.
What about turning this method into
/**
* @param \Closure(EventInterface|DecoratedEvent): array<mixed> $callback
**/
public function mapEvents(\Closure $callback): array
{
return new self(
$this->streamName,
$this->events->map($callback),
$this->expectedVersion
);
}
and then in the calling side below do sth. along the lines of
$isFirstEvent = true;
$enrichedEventsToPublish = $eventsToPublish->mapEvents(function (EventInterface|DecoratedEvent) use (&$isFirstEvent, $correlationId) {
$metadata = null;
if ($isFirstEvent) {
$metadata = $event instanceof DecoratedEvent && $event->metadata !== null ? $event->metadata->value : [];
$metadata['structureAdjustment'] = mb_strimwidth($adjustment->render() , 0, 250, '…');
$isFirstEvent = false;
}
return DecoratedEvent::create(
event: $event,
correlationId: $correlationId,
metadata: $metadata,
)
);
ok, this is is not much cleaner. But it avoids turning iterators to arrays vice versa and is a bit less dependent to inner workings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw: you can probably omit the correlation id since we should add that within nevermind, in this case that's not possibleContentRepository::handle()
with #3887
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&$isFirstEvent
:D no okay fine with me ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"along the lines of" – there's probably more bugs in the dummy code above.
BTW: DecoratedEvent
and EventMetadata
is not easy to be used.. We could extend that (in the neos/eventstore
package) a bit so that we could do
$metadata->with('newKey', $newValue);
so that we could simplify the above to sth like:
$decoratedEvent = DecoratedEvent::create(
event: $event,
correlationId: $correlationId,
);
if ($isFirstEvent) {
$decoratedEvent = $decoratedEvent->withAddedMetadata('structureAdjustment', mb_strimwidth($adjustment->render() , 0, 250, '…'));
$isFirstEvent = false;
}
return $decoratedEvent;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some initial thoughts on correlation and causation ids, happy to discuss this further!
@@ -33,4 +35,31 @@ public static function empty(): self | |||
ExpectedVersion::ANY() | |||
); | |||
} | |||
|
|||
public function withCausationOfFirstEventAndAdditionalMetaData(EventMetadata $metadata): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea in general (thus #3887) but we have to get this right:
The first event is not really the cause for the other events but the command. Our commands don't have an id yet. We could actually change that since we persist the commands.
But instead we should IMO focus on the correlation id (so that all events of one transaction are inter-connected).
The causation id does not have to be a UUID – we could e.g. add some speaking prefix, for example the command type (see #3887 (comment))
$enrichedEventsToPublish = $eventsToPublish->withCausationOfFirstEventAndAdditionalMetaData( | ||
EventMetadata::fromArray([ | ||
'structureAdjustment' => mb_strimwidth($adjustment->render() , 0, 250, '…') | ||
]) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this should look something like:
$enrichedEventsToPublish = $eventsToPublish->withCausationOfFirstEventAndAdditionalMetaData( | |
EventMetadata::fromArray([ | |
'structureAdjustment' => mb_strimwidth($adjustment->render() , 0, 250, '…') | |
]) | |
); | |
$enrichedEventsToPublish = $eventsToPublish->withCorrelationId(CorrelationId::fromString(UuidFactory::create()); |
/** @var list<EventInterface|DecoratedEvent> $restEvents */ | ||
$restEvents = iterator_to_array($this->events); | ||
if (empty($restEvents)) { | ||
return $this; | ||
} | ||
$firstEvent = array_shift($restEvents); | ||
|
||
if ($firstEvent instanceof DecoratedEvent && $firstEvent->eventMetadata) { | ||
$metadata = EventMetadata::fromArray(array_merge($firstEvent->eventMetadata->value, $metadata->value)); | ||
} | ||
|
||
$decoratedFirstEvent = DecoratedEvent::create($firstEvent, eventId: Event\EventId::create(), metadata: $metadata); | ||
|
||
$decoratedRestEvents = []; | ||
foreach ($restEvents as $event) { | ||
$decoratedRestEvents[] = DecoratedEvent::create($event, causationId: $decoratedFirstEvent->eventId); | ||
} | ||
|
||
return new EventsToPublish( | ||
$this->streamName, | ||
Events::fromArray([$decoratedFirstEvent, ...$decoratedRestEvents]), | ||
$this->expectedVersion | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's quite a lot of complexity. I don't really think that we need to handle the first event differently from the others in this case (see suggestion below) so maybe we can simplify this to a one-liner like:
return new self(
$this->streamName,
$this->events->map(fn (Event $event) => DecoratedEvent::create(
event: $event,
correlationId: $correlationId,
),
$this->expectedVersion,
);
…eAggregate ... The behavior should be fully equivalent
1e97147
to
07f429a
Compare
I found out that this line
neos-development-collection/Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php
Line 128 in 1fcdec4
This pr also adds causation ids and meta data for changes made by structure adjustments to easier debug them. See also #3887
Related: #4966
Related: #4832 (comment)
Upgrade instructions
Review instructions
Checklist
FEATURE|TASK|BUGFIX
!!!
and have upgrade-instructions